Fix requests and urllib instrumentations span name callback parameters #259
Conversation
lzchen
requested review from
a team,
toumorokoshi and
hectorhdzg
and removed request for
a team
| @@ -122,7 +122,7 @@ def _instrumented_requests_call( | |||
| method = method.upper() | |||
| span_name = "" | |||
| if name_callback is not None: | |||
| span_name = name_callback() | |||
| span_name = name_callback(method, url) | |||
There was a problem hiding this comment.
Just a thought that although this is sufficient for the vast majority of cases, I have a feeling people may want to customize the span name further later on.
For example, one cannot extracted values from the header and add them to the span name.
I don't have a great solution (besides passing in all args via **kwargs, etc), but this may come up as an issue.
There was a problem hiding this comment.
Yeah I thought of that too. For now, I think this is sufficient and we can change the parameters if the need arises.
...on/opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py
Show resolved
Hide resolved
|
QQ: why does this have the "Skip Changelog" tag? this is a new feature being added to the requests / urllib instrumentations. or at least a bugfix. |
| @@ -39,6 +39,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| ([#242](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/242)) | |||
| - `opentelemetry-instrumentation-flask` Do not emit a warning message for request contexts created with `app.test_request_context` | |||
| ([#253](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/253)) | |||
| - `opentelemetry-instrumentation-requests`, `opentelemetry-instrumentation-urllib` Fix span name callback parameters | |||
| - ([#259](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/259)) | |||
There was a problem hiding this comment.
Note that there's an extra dash on line 43 here.
There was a problem hiding this comment.
Probably will be fixed in a new pr
Addresses this comment